feat: move nil check to call sites #658
Conversation
SoulPancake
changed the title
SoulPancake
changed the title
|
@rbuckton Can you take a look at this? |
|
@SoulPancake Sorry for jumping on here for a slightly tangential issue, but I noticed that nil checks are inconsistent throughout the codebase. For example, https://github.com/microsoft/typescript-go/blob/main/internal%2Fast%2Fast.go#L15-L31 type Visitor func(*Node) bool
func visit(v Visitor, node *Node) bool {
if node != nil {
return v(node)
}
return false
}
func visitNodes(v Visitor, nodes []*Node) bool {
for _, node := range nodes {
if v(node) {
return true
}
}
return false
}
|
|
@SaadiSave Agreed, I will add the nil check in the loop |
|
@SoulPancake is there a lint to enforce nil checks? |
|
Don't think so @SaadiSave |
|
This PR has gotten out of date as main changed. Could you update it, or close it if you don't plan on working on this anymore? |
|
I will update it asap @jakebailey |
SoulPancake
force-pushed
the
ab/isFuncLikeDecrl
branch
from
699cd2d to
fd4cf87
Compare
|
@jakebailey Done, Can you take a look at this? |
|
I feel like most of these are redundant; I would think we'd only want ones that correspond to undefined checks in the original code. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a TODO comment by moving nil checks from the IsFunctionLike and IsFunctionLikeDeclaration utility functions to their call sites, improving code clarity and potentially performance by avoiding redundant nil checks.
Key changes:
- Removed internal nil checks from
IsFunctionLikeandIsFunctionLikeDeclarationfunctions - Added explicit nil checks at all call sites where these functions are used
- Updated function documentation to clarify the nil check requirement
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ast/utilities.go | Removed nil checks from IsFunctionLike and IsFunctionLikeDeclaration, added documentation comments |
| internal/checker/checker.go | Added nil checks before calling IsFunctionLike and IsFunctionLikeDeclaration functions |
| internal/checker/grammarchecks.go | Added nil checks before calling IsFunctionLikeDeclaration and IsFunctionLike functions |
| internal/checker/flow.go | Added nil check before calling IsFunctionLike function |
| internal/binder/binder.go | Added nil check before calling IsFunctionLike function |
| @@ -306,7 +306,7 @@ func (c *Checker) checkGrammarModifiers(node *ast.Node /*Union[HasModifiers, Has | |||
| parent := node.Parent | |||
|
|
|||
| if node.Kind == ast.KindTypeParameter { | |||
| if !(ast.IsFunctionLikeDeclaration(parent) || ast.IsClassLike(parent) || | |||
| if !((parent != nil && ast.IsFunctionLikeDeclaration(parent)) || ast.IsClassLike(parent) || | |||
There was a problem hiding this comment.
The nil check is only applied to the first condition in the OR expression. The call to ast.IsClassLike(parent) and other functions on subsequent lines also need nil checks for parent.
Copilot uses AI. Check for mistakes.
| @@ -2080,7 +2080,7 @@ func (c *Checker) checkGrammarStatementInAmbientContext(node *ast.Node) bool { | |||
| if node.Flags&ast.NodeFlagsAmbient != 0 { | |||
| // Find containing block which is either Block, ModuleBlock, SourceFile | |||
| links := c.nodeLinks.Get(node) | |||
| if !links.hasReportedStatementInAmbientContext && (ast.IsFunctionLike(node.Parent) || ast.IsAccessor(node.Parent)) { | |||
| if !links.hasReportedStatementInAmbientContext && (node.Parent != nil && ast.IsFunctionLike(node.Parent) || ast.IsAccessor(node.Parent)) { | |||
There was a problem hiding this comment.
The nil check is only applied to the first condition in the OR expression. The call to ast.IsAccessor(node.Parent) also needs a nil check for node.Parent.
| if !links.hasReportedStatementInAmbientContext && (node.Parent != nil && ast.IsFunctionLike(node.Parent) || ast.IsAccessor(node.Parent)) { | |
| if !links.hasReportedStatementInAmbientContext && node.Parent != nil && (ast.IsFunctionLike(node.Parent) || ast.IsAccessor(node.Parent)) { |
Copilot uses AI. Check for mistakes.
|
@jakebailey I guess these aren't needed or helpful as you mentioned, do you suggest I remove the TODO comments and remove the changes? |
|
Otherwise I am happy to close this PR @jakebailey |
This addresses
// TODO(rbuckton): Movenode != niltest to call sites@rbuckton Can you please review this
I can undo the stylistic whitespace changes in the internal/ast/utilities.gbut I think they're for the better, LMK what you think